Skip to content

Improve bounds checking in V1 transport #1009

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 3 commits into from
Closed

Improve bounds checking in V1 transport #1009

wants to merge 3 commits into from

Conversation

rheatley-pervasid
Copy link

@rheatley-pervasid rheatley-pervasid commented Mar 24, 2022 •
edited
Loading

We have seen a customer suffer a crash with the following stack trace

Description: The process was terminated due to an unhandled exception.
Exception Info: System.OverflowException
   at NetMQ.GCBufferPool.Take(Int32)
   at NetMQ.Msg.InitPool(Int32)
   at NetMQ.Core.Transports.V1Decoder.EightByteSizeReady()
   at NetMQ.Core.Transports.V1Decoder.Next()
   at NetMQ.Core.Transports.DecoderBase.Decode(NetMQ.Core.Transports.ByteArraySegment, Int32, Int32 ByRef)
   at NetMQ.Core.Transports.StreamEngine.ProcessInput()
   at NetMQ.Core.Transports.StreamEngine.Handle(Action, System.Net.Sockets.SocketError, Int32)
   at NetMQ.Core.Transports.StreamEngine.FeedAction(Action, System.Net.Sockets.SocketError, Int32)
   at NetMQ.Core.Transports.StreamEngine.InCompleted(System.Net.Sockets.SocketError, Int32)
   at NetMQ.Core.IOObject.InCompleted(System.Net.Sockets.SocketError, Int32)
   at NetMQ.Core.Utils.Proactor.Loop()
   at System.Threading.ThreadHelper.ThreadStart_Context(System.Object)
   at System.Threading.ExecutionContext.RunInternal(System.Threading.ExecutionContext, System.Threading.ContextCallback, System.Object, Boolean)
   at System.Threading.ExecutionContext.Run(System.Threading.ExecutionContext, System.Threading.ContextCallback, System.Object, Boolean)
   at System.Threading.ExecutionContext.Run(System.Threading.ExecutionContext, System.Threading.ContextCallback, System.Object)
   at System.Threading.ThreadHelper.ThreadStart()

This was on version 4.0.1.6.
The crash is in our application which has a RouterSocket bound to "tcp://127.0.0.1:9202"

I tracked down issues #225 and #388 and believe this is the same problem.
I have so far failed to reproduce the issue using NetMQ or python zmq, it isn't clear to me how to send a large enough message.

I can provoke the crash with some contrived python code

import socket
s = socket.socket(socket.AF_INET, socket.SOCK_STREAM)
s.connect(("127.0.0.1", 9202))
s.send(b'\xFF' * 9 + b'\x00' + b'\x42' * 1000000)

Taking the fix from #388 and applying to V1Decoder.cs prevents the above python code crashing NetMQ.
There is a comment in that PR - "Other decoders/encoders are worth checking out too I guess." - I assume this never happened?

Let me know your thoughts

@rheatley-pervasid
Copy link
Author

@drewnoakes anything I can do to help progress this?

@rheatley-pervasid
Copy link
Author

Oh, looks like it got fixed differently in #1030
Assume you are happy to lose support for messages >2GB and I'll close it?

@drewnoakes
Copy link
Member

Apologies for dropping the ball on this.

I'm on my phone and don't have the spec handy. If the type is unsigned then the fix here would be more comprehensive.

@drewnoakes drewnoakes closed this Jun 13, 2025
@drewnoakes drewnoakes reopened this Jun 13, 2025
@drewnoakes drewnoakes changed the title Implement similar fix to issue #225 (fixed in #388) for V1Decoder Improve bounds checking in V1 transport Jun 13, 2025
@drewnoakes
Update V1Decoder.cs
c8c8d2b 
Explicitly touching the line that was changed on another branch to force merge resolution in the web interface.
@drewnoakes
Merge branch 'master' into v1decoder-EightByteSizeReady-overflow
26fe643
@drewnoakes
Copy link
Member

Oh, looks like it got fixed differently in #1030 Assume you are happy to lose support for messages >2GB and I'll close it?

So looking at this PR again (sorry again for the delay) I am wondering if this is any different in practice to the previous change that checked for negative values.

While this will correctly pull a payload length greater than 2GB, it then goes on to validate that it's no longer than 0x7FFFFFC7, which is actually less than int.MaxValue. I don't know where that constant comes from, and it seems more restrictive than what's on the main branch now.

@drewnoakes
Copy link
Member

I don't think this changes any behaviour, since #1030 was merged, so will close this out.

@drewnoakes drewnoakes closed this Jun 14, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@somdoron somdoron somdoron approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@rheatley-pervasid @drewnoakes @somdoron